-
Notifications
You must be signed in to change notification settings - Fork 16
refactor: common vitest configurations #1107
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
View your CI Pipeline Execution ↗ for commit 7008a48
☁️ Nx Cloud last updated this comment at |
@code-pushup/ci
@code-pushup/cli
@code-pushup/core
@code-pushup/create-cli
@code-pushup/models
@code-pushup/nx-plugin
@code-pushup/coverage-plugin
@code-pushup/eslint-plugin
@code-pushup/js-packages-plugin
@code-pushup/jsdocs-plugin
@code-pushup/lighthouse-plugin
@code-pushup/typescript-plugin
@code-pushup/utils
@code-pushup/models-transformers
commit: |
Code PushUp🤨 Code PushUp report has both improvements and regressions – compared current commit 5a5fcfa with previous commit 33714e2. 🕵️ See full comparison in Code PushUp portal 🔍 🏷️ Categories👎 3 groups regressed, 👍 4 audits improved, 👎 4 audits regressed, 14 audits changed without impacting score🗃️ Groups
18 other groups are unchanged. 🛡️ Audits
588 other audits are unchanged. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for cleaning up our test config mess :)
I left some comments.
- please add a proper PR description that contains the scope and planned changes
- move the config code under a Nx project in
testing/vitest-setup
. I updated the issue accordingly.
const testTimeout = | ||
kind === 'unit' | ||
? TEST_TIMEOUTS.MEDIUM | ||
: kind === 'int' | ||
? TEST_TIMEOUTS.LONG | ||
: TEST_TIMEOUTS.E2E; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's avoid conditionals in tests. It makes the test code harder to read and adds noise.
['unit', setupPresets.unit.base, createUnitConfig], | ||
['int', setupPresets.int.base, createIntConfig], | ||
['e2e', setupPresets.e2e.base, createE2eConfig], | ||
] as const)('%s config creation', (kind, baseSetupFiles, createFn) => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The each block for describe is introducing conditional checks in the it blocks as well as const. It further makes the tests harder to read. Let's avoid conditionals in tests as good as possible.
If you want to test basic values you could have a separate it block using the each for static things like defaults that are always the same. In the specific cases you can use expect.objectContaining
to keep is focused.
export function createVitestConfig( | ||
options: VitestConfigFactoryOptions, | ||
overrides: VitestOverrides = {}, | ||
): ViteUserConfig { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I suggest excluding overrides from the logic and push it to the user. This avoids complexity and reduces maintenance efforts to sync with vitest changes. WDYT @matejchalk
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please let me know which direction we should follow:
- Easier override handling ( current solution ) - in this case we take care of most of the overrides but on the other hand we are exposed to vitest changes (if the happen) and we need to keep up it them
- We go easy way and move transfer the "burden" of overrides to end consumer / user.
Both approaches have pros and cons. I don't feel like I'm the right person to make a decision here 😅
projectKey: string; | ||
kind: TestKind; | ||
projectRootUrl: URL; | ||
cacheDirName: string; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
you could derive the cacheDirName
from the projectKey
. Internally if needed you can modify if needed e.g. cache-${projectKey}
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is it suggestion or request change 😄 ?
Deriving cacheDirName
from projectKey
in form: cache-${projectKey}
sounds ok but makes me wonder, aren't we making some hidden bond between project key and cache directory?
Maybe we could consider cacheDirName
as optional which fallbacks to cache-${projectKey}
if not specified?
Let me know @BioPhoton
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
EDIT:
We already using such bond for coverage, let's do the same for cache. I'll update it
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe we could consider cacheDirName as optional which fallbacks to ...
Yes better!
cache-${projectKey} if not specified?
the "cache-" was just something to give example. In general the cache key (and directory) should be unique across the repository. That is the reason we are about it in the first place.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Method you highlighted in comment is kind of "internal", the fallback is done in line 50
Is that ok acceptable or do you suggest other approach to that?
testing/test-setup-config/README.md
Outdated
### Parameters | ||
|
||
- `projectKey`: Used for cache and coverage directory naming | ||
- `projectRoot`: Required path/URL to the project root for resolving all paths | ||
- Standard Vitest configuration options can be provided in the overrides parameter |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is covered by the types. You could add TSDocs and remove it here.
testing/test-setup-config/README.md
Outdated
### Defaults | ||
|
||
**Common**: `reporters: ['basic']`, `globals: true`, `environment: 'node'`, `pool: 'threads'` with `singleThread: true`, alias from tsconfig paths | ||
|
||
**Coverage**: Unit/Int enabled (reports to `<projectRoot>/packages/<project>/.coverage`), E2E disabled. Excludes `['mocks/**', '**/types.ts']` | ||
|
||
**Global setup**: Unit/Int use `['<projectRoot>/global-setup.ts']`, E2E none by default | ||
|
||
**Include patterns**: Unit `src/**/*.unit.test.*`, Int `src/**/*.int.test.*`, E2E `tests/**/*.e2e.test.*` | ||
|
||
### Setup files | ||
|
||
**Automatic inclusion**: Unit (console mocking, cleanup, UI/filesystem mocks, basic matchers), Int (console mocking, cleanup), E2E (cleanup only) | ||
|
||
**Custom setup files**: ⚠️ Specifying `setupFiles` in overrides will completely replace the defaults. To extend the default list, manually combine them with `setupPresets`: | ||
|
||
- `setupPresets.unit.{base, git, portalClient, matchersCore, matcherPath}` | ||
- `setupPresets.int.{base, cliui, fs, git, portalClient, matcherPath, chromePath}` | ||
- `setupPresets.e2e.{base}` | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The defaults and setup files could be documented in unit tests. The example section could show the usage directly.
}, | ||
{ | ||
test: { | ||
testTimeout: 60_000, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this could be a default for the e2e target
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I checked other files and there are many different values, like 20k, 40k, 80k, 60k, 120k.
Just making sure that we want to have 60k as default?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmmm.... I would put 20 as default. Or if there are more projects with 40 lets go with 40. And for the projects with higher or lower times go with a override.
Did not realise this is already an overwrite..
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I checked that and:
Timeout | Occurences |
---|---|
20s | 6 |
40s | 1 |
60s | 1 |
80s | 2 |
120s | 1 |
I suggest then use 20s as default (20_000ms)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the changes! Nice testing tool! There is Readme, and tests, lovely.
I left some first comments and will wait for one feedback on the override logic for further comments.
9433ca5
to
7008a48
Compare
Centralize and Standardize Vitest Configurations
What Changed
Benefits
The new system provides
createUnitConfig
,createIntConfig
, andcreateE2eConfig
factory functions that automatically handle common configuration patterns while still allowing customization through override parameters.Relates to first part of #1065